Closed
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors routing and proxy request handling to remove the legacy “return route id” concept, make the target host explicit (via a new Host type), and improve test/debug ergonomics around stream keys and protocol packet introspection.
Changes:
- Introduces
sub_lib::host::Hostand threads it through route queries/responses and proxy request payload creation. - Removes return-route-id plumbing from
Route/Neighborhood/tests and deletes related message types/subscriptions. - Enhances proxy protocol packs with
describe_packet()and extendsTtlHashMapwith a retire callback +remove()API.
Reviewed changes
Copilot reviewed 35 out of 37 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| node/src/test_utils/recorder.rs | Removes recording support for deprecated proxy-server return-route message subscription. |
| node/src/test_utils/mod.rs | Updates route/test helpers to include host/TLS context; removes return-route-id helper; adjusts message type helper signature. |
| node/src/sub_lib/ttl_hashmap.rs | Adds retire-closure behavior and remove(); expands unit tests accordingly. |
| node/src/sub_lib/stream_key.rs | Makes “meaningless” stream keys random; adds a test-only constructor for deterministic keys. |
| node/src/sub_lib/stream_handler_pool.rs | Renames TransmitDataMsg.sequence_number to sequence_number_opt. |
| node/src/sub_lib/sequence_buffer.rs | Adapts to TransmitDataMsg.sequence_number_opt rename. |
| node/src/sub_lib/route.rs | Removes return-route-id encoding/decoding and related debug/test logic. |
| node/src/sub_lib/proxy_server.rs | Makes ClientRequestPayload_0v1.target_hostname mandatory and removes AddReturnRouteMessage + subscription. |
| node/src/sub_lib/neighborhood.rs | Replaces optional hostname with Host in route queries and stores host in route responses; adds ExpectedService accessors; removes return-route-id from ExpectedServices. |
| node/src/sub_lib/mod.rs | Exposes new host module. |
| node/src/sub_lib/migrations/client_request_payload.rs | Updates payload migration to treat target_hostname as required (non-optional). |
| node/src/sub_lib/http_packet_framer.rs | Fixes logger name to match the component. |
| node/src/sub_lib/host.rs | Adds new Host type with Display and tests. |
| node/src/sub_lib/hopper.rs | Updates tests to pass explicit StreamKey into meaningless message helper. |
| node/src/sub_lib/dispatcher.rs | Renames fields to *_opt for clarity and updates debug output + tests. |
| node/src/stream_reader.rs | Renames reception_port/sequence_number fields to *_opt and propagates into messages/logging/tests. |
| node/src/stream_handler_pool.rs | Updates logic/tests for sequence_number_opt and reception_port_opt naming. |
| node/src/proxy_server/tls_protocol_pack.rs | Moves Host import to sub_lib, adds describe_packet() and detailed TLS packet descriptions + tests. |
| node/src/proxy_server/server_impersonator_tls.rs | Updates DNS-failure response signature to take a required server name. |
| node/src/proxy_server/server_impersonator_http.rs | Updates DNS-failure response signature to take a required server name and removes “unspecified” variant test. |
| node/src/proxy_server/protocol_pack.rs | Removes duplicate Host struct, adds describe_packet() to ProtocolPack, and updates from_ibcd() to use reception_port_opt. |
| node/src/proxy_server/http_protocol_pack.rs | Moves Host import to sub_lib, adds describe_packet() with human-readable request/response summaries + tests. |
| node/src/proxy_server/client_request_payload_factory.rs | Accepts optional host from history, requires a resolved host, and uses protocol packs’ packet descriptions in error messages; updates/extends tests. |
| node/src/proxy_client/stream_reader.rs | Updates tests to avoid assuming deterministic “meaningless” stream keys. |
| node/src/proxy_client/stream_handler_pool.rs | Removes optional hostname handling; updates logic/tests to assume target_hostname is always present. |
| node/src/proxy_client/stream_establisher.rs | Updates tests for mandatory hostname and non-deterministic meaningless stream keys. |
| node/src/proxy_client/mod.rs | Updates tests for mandatory hostname and updated route helper signatures. |
| node/src/neighborhood/mod.rs | Threads Host through route building/undesirability logic and updates many tests for removed return-route-id and new route length expectations. |
| node/src/hopper/routing_service.rs | Updates tests for new route helper signatures and renamed *_opt fields. |
| node/src/hopper/mod.rs | Updates tests for new route helper signatures and make_meaningless_message_type(stream_key) signature. |
| node/src/hopper/live_cores_package.rs | Updates tests for removed return-route-id behavior and explicit stream keys in meaningless message creation. |
| node/src/hopper/consuming_service.rs | Updates tests for renamed *_opt fields and explicit stream keys in meaningless message creation. |
| node/src/dispatcher.rs | Updates tests for renamed *_opt fields and sequence_number_opt. |
| multinode_integration_tests/tests/self_test.rs | Updates integration tests for explicit stream keys in meaningless message creation. |
| multinode_integration_tests/tests/connection_termination_test.rs | Removes return-route-id extraction/usage and updates request/report helpers accordingly. |
| .gitignore | Ignores Copilot-related IDE migration artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| } else { | ||
| error!(logger, "{}", e); | ||
| } | ||
| error!(logger, "{}", e); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.